-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Smithy Document Shape Support #310
Conversation
...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java
Show resolved
Hide resolved
...n/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ShapeValueGenerator.java
Show resolved
Hide resolved
...n/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ShapeValueGenerator.java
Outdated
Show resolved
Hide resolved
...en/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/StructureGenerator.java
Show resolved
Hide resolved
@@ -411,7 +411,7 @@ protected final void generateDeserFunction( | |||
BiConsumer<GenerationContext, Shape> functionBody | |||
) { | |||
SymbolProvider symbolProvider = context.getSymbolProvider(); | |||
GoWriter writer = context.getWriter(); | |||
GoWriter writer = context.getWriter().get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this a lot. Did this method get changed to be Optional? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ProtocolGenerator context is kind of a bit of a grab bag currently, in some instances a writer is to be expected, in other instances the delegator that is stored on the context is used and no writer is expected. This was just to make that contact a bit clearer that it may not be there.
This was just something that I noticed when I was refactoring context to make it immutable after construction.
...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/ProtocolUtils.java
Outdated
Show resolved
Hide resolved
document/document.go
Outdated
"strconv" | ||
) | ||
|
||
type SmithyDocumentMarshaler interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these kinds of interfaces generally have godocs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, next revision will include documentation, just wanted to get this out for initial feedback on the overall implementation.
return v.Interface() | ||
} | ||
|
||
func IsZeroValue(v reflect.Value) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this here to prevent zero values from being serialized? If so, I don't think that's the correct behavior for document types. They're meant to carry whatever input they were given, so I expect if someone populates a 0
or []
or whatever, then it's serialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is here to detect if a value is a zero-value, this doesn't prevent serialization of the zero value unless the user instructed the document marshaler via a struct tag likedocument:",omitempty"
, where omitempty indicates that zero-values should not be serialized.
...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java
Outdated
Show resolved
Hide resolved
/** | ||
* Generates the Smithy document type for the service client. | ||
*/ | ||
public final class ProtocolDocumentGenerator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this class need to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these exported constants are needed so that protocol implementation parts make symbol references to the types.
import software.amazon.smithy.model.shapes.DocumentShape; | ||
|
||
/** | ||
* Generates the Smithy document type for the service client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give more info on what this generates? Do you generate a document type per type defined in the model? Do you generate a single document type used by every document type, and this mostly just generates the serde code specific to the protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add onto this, it would be generally nice to have example usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add some clarifying code examples sections to this class documentation. To summarize what is being created here: Each service client generates its own document type interface definition. This ensures that document types that are sent or received from the service are bound to the client package and it's associated client protocol. This prevents the situation where a user receives a set of protocol document bytes in service client A and tries to round-trip it by plugging it directly into service client B (which may be a different protocol and where the raw captured bytes can't be used). User who want to plug a document from service client A into client B would need to first unmarshal the document into a Go type, and then pass that Go type to the NewLazyDcoument
constructor in the service client B package.
...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java
Outdated
Show resolved
Hide resolved
...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java
Show resolved
Hide resolved
codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/CodegenVisitor.java
Outdated
Show resolved
Hide resolved
import software.amazon.smithy.model.shapes.DocumentShape; | ||
|
||
/** | ||
* Generates the Smithy document type for the service client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add onto this, it would be generally nice to have example usage.
...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java
Show resolved
Hide resolved
...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java
Outdated
Show resolved
Hide resolved
...n/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ShapeValueGenerator.java
Show resolved
Hide resolved
...n/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ShapeValueGenerator.java
Outdated
Show resolved
Hide resolved
7fc9824
to
09269c5
Compare
cb032bb
to
a87ef15
Compare
I believe all outstanding feedback has been capture in the |
...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java
Show resolved
Hide resolved
Rebased on |
Adds support for Smithy Document Shape Types for JSON protocols. Depends on: aws/smithy-go#310
Adds support for Smithy Document Shape Types for JSON protocols. Depends on: aws/smithy-go#310
Adds support for Smithy Document shapes and supporting types for protocols to implement support.
AWS Protocol Support: aws/aws-sdk-go-v2#1320
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.